Improved support for string enums #4147

RunDevelopment · 2024-10-08T13:04:38Z

Another string enum PR.

Changes:

Fixed Typescript definition not generated for string enums #2153. Typescript types are now generated for string enums.
String enums now support the skip_typescript and js_name options, just like C-style enums.
Bindings now generate a non-exported array for the string enum variants and reference the array when converting to and from WASM.
Improved error messages. This together with 1. fixes Implementing String Enums #3057.

Everything (except skip_typescript) is tested in crates/cli/tests/reference/enums.rs.

1. Typescript types

String enums now generate TS types. Example:

/// The name of a color.
#[wasm_bindgen]
#[derive(PartialEq, Debug)]
pub enum ColorName {
    Green = "green",
    Yellow = "yellow",
    Red = "red",
}

/**
 * The name of a color.
 */
export type ColorName = "green" | "yellow" | "red";

I used a union type of string literal to represent the fact that there is no exported object on the JS side.

A const enum also would have been possible. In fact, this would have been the better option in a way, because it would allow users to use string enums just like regular enums, e.g. ColorName.Green. However, const enums do not allow using string literals directly (e.g. const _: ColorName = "green" does not type check), and because const enums are not supported by some bundlers.

2. `js_name` and `skip_typescript`

My goal was to make string enums and C-style enums are similar as possible, so I added support for these 2 attributes while I was at it. (In general, the code handling the implementation of string enums and C-style enums is now quite similar. Maybe it could be more unified in the future, but maybe that goes too far.)

skip_typescript is important to support, since users might have worked around the lack of string enum types with typescript_custom_section before (e.g. I did that in my project). While most of these custom type defs can be removed now, some users may wish to keep their types, and this option allows them to do so.

js_name is here for consistency and because it's useful.

3. Bindings

Instead of creating an array with all string enum variants for each conversion, I changed the bindings to create one global array. So instead of ["a", "b", "c"][index] for each conversion, it now generates _StringEnum_values[index].

I used _{name}_values as the name of the constant, because I wanted to avoid name collisions. I wasn't sure what the best way of doing this is, so I just made up a naming scheme that I feel is unlikely to be used by WASM bindgen users.

4. Improved error messages

As pointed out in #3057, the error messages for mixed string-and-C-style enums were not ideal.

I fixed this by changing how string enums are detected. Now, when an enum contains one variant with a string discriminate, it's assumed to be a string enum. This makes it easier to give relevant error messages.

I also changed a few enum error messages that make it sound like only C-style enums are supported.

daxpedda · 2024-10-08T13:31:39Z

I used _{name}_values as the name of the constant, because I wanted to avoid name collisions. I wasn't sure what the best way of doing this is, so I just made up a naming scheme that I feel is unlikely to be used by WASM bindgen users.

Just prefix it with __wbindgen and a namespace. E.g. __wbindgen_enum_*.

Thank you for looking into this!

RunDevelopment · 2024-10-08T14:24:21Z

Just prefix it with __wbindgen and a namespace. E.g. __wbindgen_enum_*.

Thank you for the suggestion. I used your __wbindgen_enum_*.

After reading through the comments in #3057, I noticed that I didn't correctly generate the types for non-public string enums.

Aside from that, I also fixed the error messages mentioned in #3057 to resolve the issue.

RunDevelopment · 2024-10-08T14:40:55Z

Btw, seems like there's a race condition in raytrace_parallel (this is unrelated to this PR). Here's the output:

running 2 tests
test raytrace_parallel ... FAILED
test wasm_audio_worklet ... ok

failures:

error: test failed, to rerun pass `-p example-tests --test shell`
---- raytrace_parallel stdout ----
thread 'raytrace_parallel' panicked at /home/runner/work/wasm-bindgen/wasm-bindgen/crates/example-tests/src/lib.rs:467:25:
called `Result::unwrap()` on an `Err` value: ()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    raytrace_parallel

daxpedda

Missing a changelog entry, otherwise LGTM!
Thank you for doing this, great work!

Its pretty weird, that C-style enums are exported in JS with a frozen object but string enums aren't at all. Not sure about the history here, but intuitively I would also not export C-style enums as well. Maybe something to consider for the next breaking change.

(raytrace_parallel and a couple of other tests in the job spuriously fail regularly, feel free to ignore it, I can just restart these jobs manually until they succeed)

RunDevelopment · 2024-10-08T20:43:27Z

Missing a changelog entry

Will add.

Its pretty weird, that C-style enums are exported in JS with a frozen object but string enums aren't at all.

The object is just how TypeScript does enums. Here's an example where you can see the JS output of tsc for a simple enum. The only difference to WASM bindgen is that TS doesn't freeze the object.

Not sure about the history here, but intuitively I would also not export C-style enums as well. Maybe something to consider for the next breaking change.

If you're suggesting to remove the exported frozen object for C-style, then please don't. That would make the bindings for C-style enums completely useless. I mean, try using the following function on the JS side:

#[wasm_bindgen]
pub fn foo(a: SomeEnum) {}

#[wasm_bindgen]
pub enum SomeEnum {
    A = 123,
    B = 624,
    C = 24891,
}

I don't want to write foo(624) in JS. I want to be able to write foo(SomeEnum.B) and that can only be achieved with an object for SomeEnum at runtime.

The only reason why string enums are useful without a frozen object is that JS/TS devs like using string literals instead of TS enums in certain places. Here's an example of this in NodeJS' crypto API. This can create pretty nice strongly-typed APIs like hash.digest("base64").

That all said, C-style enums and string enums are still handled inconsistent as you noted. C-style enums are proper TS enums, but string enums are type only. I think one way to resolve this would be to make both TS enums by default and support type-only string enums via an option (e.g. #[wasm_bindgen(type_only)]). This would also open up the path for mixed C-style and string enums. E.g.

#[wasm_bindgen]
pub enum Foo {
  A = 1,
  B = 3,
  C = "foo",
}

could become

// .d.ts
export enum Foo {
  A = 1,
  B = 3,
  C = "foo",
};

// .js
export const Foo = Object.freeze({
  A: 1, "1": "A",
  B: 3, "3": "B",
  C: "foo", "foo": "C", 
});

On the ABI side, mix enums would still use u32, just like C-style enums and string enums.

Well, it's a breaking change for string enums, so whatever.

feel free to ignore it, I can just restart these jobs manually until they succeed

No worries. I can just push an empty commit to trigger a re-run.

daxpedda · 2024-10-08T21:15:19Z

Its pretty weird, that C-style enums are exported in JS with a frozen object but string enums aren't at all.

The object is just how TypeScript does enums. Here's an example where you can see the JS output of tsc for a simple enum. The only difference to WASM bindgen is that TS doesn't freeze the object.

Not sure about the history here, but intuitively I would also not export C-style enums as well. Maybe something to consider for the next breaking change.

If you're suggesting to remove the exported frozen object for C-style, then please don't.

Just to clarify: as a maintainer of wasm-bindgen I don't make judgement calls on JS/TS exports because I don't have any expertise in that area. Please bear with me while I come to very naive conclusions while learning about it.

In any case, I abstain from doing any changes in this area without some significant input from others who know what they are talking about!

The reason why I'm suggesting removing the JS export is because enums inherently do not possess a JS equivalent. Generating JS bindings from WebIDL also has no conversion, which is something I would rather follow then doing some custom conversion.

I don't want to write foo(624) in JS. I want to be able to write foo(SomeEnum.B) and that can only be achieved with an object for SomeEnum at runtime.

That's very understandable, but I think this should be explicitly opted into instead of doing our own custom conversion implicitly.

daxpedda

Thank you!

RunDevelopment · 2024-10-08T23:16:49Z

Please bear with me while I come to very naive conclusions while learning about it.

No worries. I just wanted to explain why the current state, despite being inconsistent, is useful.

RunDevelopment added 4 commits October 8, 2024 14:31

Improved bindings for string enums again

4f501da

Merge branch 'main' into improved-bindings-for-string-enums

1477c81

I don't like raw.js

9d4f5ad

Updated hash

3c5f44e

RunDevelopment marked this pull request as draft October 8, 2024 13:17

Support private string enums

a42782d

RunDevelopment added 4 commits October 8, 2024 16:12

Better error messages

a55939c

Use suggested prefix for internal array variable

5b02e44

Update schema hash

0be8fb6

Use same definition for "public" as for C-style enums

4efc729

RunDevelopment marked this pull request as ready for review October 8, 2024 14:24

Pls tell me it's a race condition

64be5fd

daxpedda requested changes Oct 8, 2024

View reviewed changes

RunDevelopment added 2 commits October 8, 2024 22:52

Updated changelog

f8b7e9a

Merge branch 'main' into improved-bindings-for-string-enums

a4fba63

RunDevelopment requested a review from daxpedda October 8, 2024 20:57

daxpedda approved these changes Oct 8, 2024

View reviewed changes

daxpedda merged commit 0be2fe5 into rustwasm:main Oct 8, 2024
41 checks passed

RunDevelopment deleted the improved-bindings-for-string-enums branch October 8, 2024 22:49

This was referenced Oct 10, 2024

Unnecessary Typescript definitions are created for web_sys string enums #4163

Closed

Revert TS string enum generation #4174

Merged

This was referenced Oct 11, 2024

Improved string enum typing try 2 #4180

Merged

Only generate Js values for string enums if used #4193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved support for string enums #4147

Improved support for string enums #4147

RunDevelopment commented Oct 8, 2024 •

edited

Loading

daxpedda commented Oct 8, 2024 •

edited

Loading

RunDevelopment commented Oct 8, 2024

RunDevelopment commented Oct 8, 2024 •

edited

Loading

daxpedda left a comment •

edited

Loading

RunDevelopment commented Oct 8, 2024

daxpedda commented Oct 8, 2024

daxpedda left a comment

RunDevelopment commented Oct 8, 2024

Improved support for string enums #4147

Improved support for string enums #4147

Conversation

RunDevelopment commented Oct 8, 2024 • edited Loading

1. Typescript types

2. js_name and skip_typescript

3. Bindings

4. Improved error messages

daxpedda commented Oct 8, 2024 • edited Loading

RunDevelopment commented Oct 8, 2024

RunDevelopment commented Oct 8, 2024 • edited Loading

daxpedda left a comment • edited Loading

Choose a reason for hiding this comment

RunDevelopment commented Oct 8, 2024

daxpedda commented Oct 8, 2024

daxpedda left a comment

Choose a reason for hiding this comment

RunDevelopment commented Oct 8, 2024

RunDevelopment commented Oct 8, 2024 •

edited

Loading

2. `js_name` and `skip_typescript`

daxpedda commented Oct 8, 2024 •

edited

Loading

RunDevelopment commented Oct 8, 2024 •

edited

Loading

daxpedda left a comment •

edited

Loading